feat(icos): First phase of the firewall setup feature (DRE-258) #1451

DFINITYManu · 2024-09-11T15:58:01Z

Per https://dfinity.atlassian.net/browse/DRE-258 .

We have successfully loaded rules of well-formed /boot/config/firewall.json in HostOS and GuestOS.

Details:
We would like to allow the node operator / provider to modify the firewall rules in order to allow incoming traffic to their nodes. This can be used to get the ability to fetch logs and metrics from the nodes in the same DC.

To achieve this, the node provider should add a new config file named firewall.json in the SetupOS config. This file is then propagated to the other parts of the IC-OS stack.

Note that the firewall rules can only be configured during node (re)deployment, in this phase. In phase 2, it will become possible to reconfigure already running nodes. Phase 2 will be more effort than phase 1.

rs/ic_os/config/src/types.rs

ic-os/setupos/data/deployment.json.template

ic-os/components/hostos-scripts/generate-guestos-config/generate-guestos-config.sh

ic-os/components/setupos-scripts/devices.sh

rs/ic_os/config/src/types/firewall.rs

rs/ic_os/config/src/firewall_json.rs

rs/ic_os/config/src/lib.rs

rs/ic_os/utils/Cargo.toml

ic-os/docs/Network-Configuration.adoc

DFINITYManu · 2024-09-13T18:35:29Z

We don't yet do anything with the firewall.json file, but all this work, already, is enormous just to pass a silly config file down the pipe. Horrible. I am so looking forward to your work @andrewbattat enabling the use of the configuration structures making this job enormously easier and deleting so much code from our codebase.

rs/ic_os/config/src/main.rs

github-actions · 2024-09-26T10:00:16Z

Vulnerable dependency information
The dependency-check job for the MR has new findings. Please update or remove these dependencies or obtain a commit exception from product security.

The findings are:
[Finding(repository='ic', scanner='BAZEL_RUST', vulnerable_dependency=Dependency(id='https://crates.io/crates/mio', name='mio', version='0.8.10', fix_version_for_vulnerability={'https://rustsec.org/advisories/RUSTSEC-2024-0019': ['>=0.8.11', '<0.7.2']}), vulnerabilities=[Vulnerability(id='https://rustsec.org/advisories/RUSTSEC-2024-0019', name='RUSTSEC-2024-0019', description='Tokens for named pipes may be delivered after deregistration', score=-1, risk_note=' ')], first_level_dependencies=[], projects=[], risk_assessor=[], risk=None, owning_teams=[], patch_responsible=[], due_date=None, score=-1, more_info=None), Finding(repository='ic', scanner='BAZEL_RUST', vulnerable_dependency=Dependency(id='https://crates.io/crates/rsa', name='rsa', version='0.9.2', fix_version_for_vulnerability={}), vulnerabilities=[Vulnerability(id='https://rustsec.org/advisories/RUSTSEC-2023-0071', name='RUSTSEC-2023-0071', description='Marvin Attack: potential key recovery through timing sidechannels', score=5, risk_note=' ')], first_level_dependencies=[], projects=[], risk_assessor=[], risk=None, owning_teams=[], patch_responsible=[], due_date=None, score=5, more_info=None)]

venkkatesh-sekar · 2024-09-26T13:39:43Z

Vulnerable dependency information

This was due to a mixup in testing. Should be resolved now.

DFINITYManu · 2024-10-03T14:06:19Z

@andrewbattat for some reason Github refuses to show me the thread where you and I were chatting about the empty firewall.json file. I still need your stamp in order to merge though. What do we do?

andrewbattat · 2024-10-03T14:43:46Z

@andrewbattat for some reason Github refuses to show me the thread where you and I were chatting about the empty firewall.json file. I still need your stamp in order to merge though. What do we do?

Here it is: #1451 (comment)

I unresolved a few threads, and I'll do another review later today.

…g because who knows thought this was a good idea.

… options.

…cified.

ic-os/components/setupos-scripts/check-network.sh

andrewbattat · 2024-10-03T15:51:02Z

rs/ic_os/os_tools/setupos_tool/src/main.rs

@@ -125,6 +138,21 @@ pub fn main() -> Result<()> {
            println!("{}", to_cidr(ipv6_address, network_info.ipv6_subnet));
            Ok(())
        }
+        Some(Commands::CheckFirewallConfig { firewall_file }) => {
+            let _config_ = firewall_json::get_firewall_rules_json_or_default(


nit: omit the unused _config variable

Suggested change

let _config_ = firewall_json::get_firewall_rules_json_or_default(

firewall_json::get_firewall_rules_json_or_default(

That may actually get the compiler to omit the call altogether, since "it has no effects".

LMK if we have to fix anyway.

andrewbattat · 2024-10-03T15:57:17Z

ic-os/components/setupos-scripts/check-network.sh

+        if [ -f "${DEFAULT_FIREWALL_FILE}" ]; then
+            echo "* Checking firewall rules in ${DEFAULT_FIREWALL_FILE}..."
+            /opt/ic/bin/setupos_tool check-firewall-config "${DEFAULT_FIREWALL_FILE}" >/dev/null || {
+                ret=$?
+                echo >&2 "Failed to parse default firewall rules file ${DEFAULT_FIREWALL_FILE}."
+                return ${ret}
+            }
+        fi
+    fi


We use log_and_halt_installation_on_error to handle errors (see the rest of this file)

Also, can we not redirect the output from the check-firewall-config to /dev/null/?

Suggested change

if [ -f "${DEFAULT_FIREWALL_FILE}" ]; then

echo "* Checking firewall rules in ${DEFAULT_FIREWALL_FILE}..."

/opt/ic/bin/setupos_tool check-firewall-config "${DEFAULT_FIREWALL_FILE}" >/dev/null || {

ret=$?

echo >&2 "Failed to parse default firewall rules file ${DEFAULT_FIREWALL_FILE}."

return ${ret}

}

fi

fi

if [ -f "${DEFAULT_FIREWALL_FILE}" ]; then

echo "* Checking firewall rules in ${DEFAULT_FIREWALL_FILE}..."

/opt/ic/bin/setupos_tool check-firewall-config "${DEFAULT_FIREWALL_FILE}"

log_and_halt_installation_on_error "${?}" "Failed to parse default firewall rules file ${DEFAULT_FIREWALL_FILE}."

I went and added that wherever I remembered that was necessary. If I missed one, I would appreciate an honest look on that.

andrewbattat · 2024-10-03T16:03:20Z

ic-os/components/setupos-scripts/setup-hostos-config.sh

@@ -26,6 +26,20 @@ function copy_config_files() {
        log_and_halt_installation_on_error "1" "Configuration file 'config.ini' does not exist."
    fi

+    if [ -v FIREWALL_FILE ]; then


What is the purpose of the FIREWALL_FILE ev? Why don't we just copy over /var/ic/config/firewall.json if it exists?

andrewbattat · 2024-10-03T16:11:32Z

rs/ic_os/config/src/firewall_json.rs

+fn get_firewall_rules_json(firewall_file: &Path) -> Result<Vec<FirewallRule>, FirewallRulesError> {
+    let file = match File::open(firewall_file) {
+        Ok(file) => file,
+        Err(e) => {
+            return Err(FirewallRulesError::IOError((
+                firewall_file.to_path_buf(),
+                e,
+            )))
+        }
+    };
+    match serde_json::from_reader(&file) {
+        Ok(val) => Ok(val),
+        Err(e) => Err(FirewallRulesError::ParseError((
+            firewall_file.to_path_buf(),
+            e,
+        ))),
+    }
+}


Can this code be simplified using ? and map_error

fn get_firewall_rules_json(firewall_file: &Path) -> Result<Vec<FirewallRule>, FirewallRulesError> { let file = File::open(firewall_file).map_err(|e| { FirewallRulesError::IOError((firewall_file.to_path_buf(), e)) })?; serde_json::from_reader(&file).map_err(|e| { FirewallRulesError::ParseError((firewall_file.to_path_buf(), e)) }) }

andrewbattat · 2024-10-03T16:18:08Z

rs/ic_os/config/src/firewall_json.rs

+    macro_rules! bad_rules_must_be_bad {
+        ($text:literal) => {
+            let temp_file = temp_fixture($text)?;
+            let outp = get_firewall_rules_json(temp_file.path());
+            assert!(outp.is_err());
+            Ok(())
+        };
+    }


Suggested change

macro_rules! bad_rules_must_be_bad {

($text:literal) => {

let temp_file = temp_fixture($text)?;

let outp = get_firewall_rules_json(temp_file.path());

assert!(outp.is_err());

Ok(())

};

}

fn check_invalid_firewall_rule(text: &str) -> Result<()> {

let temp_file = temp_fixture(text)?;

let outp = get_firewall_rules_json(temp_file.path());

assert!(outp.is_err());

Ok(())

}

I find a helper function much more readable here

andrewbattat · 2024-10-03T16:19:27Z

rs/ic_os/config/src/firewall_json.rs

+use std::path::Path;
+use std::path::PathBuf;


Suggested change

use std::path::Path;

use std::path::PathBuf;

use std::path::{Path,PathBuf};

The setup routine uses /dev/vda to set up HostOS and GuestOS. Unfortunately, that drive contains the setup partition, which is happily destroyed by the installer as the installer proceeds. When time comes to reboot, a bunch of gobbledygook is printed to the console, and the system just hangs uselessly after that. To work around that issue and permit installation of the OS in the QEMU environment, we create an empty disk (sparse) and use that as the first disk during QEMU invocation. This solves the issue of SetupOS destroying itself prior to reboot. We also increase startup performance by avoiding the copy of the tarball that contains the SetupOS image, and by asking tar to be sparse with data copies whenever it can be. Finally, we attempt to clean up the folder upon completion of the execution. This avoids large files remaining around after setup. It's truly space-efficient eh! ``` manuel@devenv-manuel:~$ du -h /tmp/tmp.FO1kRm0gqF.qemu-launch-remote-vm/* 2.5G /tmp/tmp.FO1kRm0gqF.qemu-launch-remote-vm/disk.img 1.6G /tmp/tmp.FO1kRm0gqF.qemu-launch-remote-vm/target.img ```

In *virtualized, non-accelerated QEMU*, the write to disk goes like this: ``` root@SetupOS:~# /opt/ic/bin/install-hostos.sh install-hostos.sh - Start * Installing HostOS disk-image... * HostOS will be deployed to /dev/vda * Temporarily extracting the HostOS image to RAM; please stand by for a few seconds * Writing the HostOS image to /dev/vda 107088969728 bytes (107 GB, 100 GiB) copied, 188 s, 570 MB/s 25650+1 records in 25650+1 records out 107585994752 bytes (108 GB, 100 GiB) copied, 190.092 s, 566 MB/s * Configuring EFI... * Resizing partition... install-hostos.sh - End (00:04:28) ```

…sults interactively after installation.

Bownairo · 2024-10-03T22:02:49Z

ic-os/components/setupos-scripts/install-guestos.sh

+    # dd will detect nulls in chunks of 4M and sparsify the writes.
+    # In *non-KVM-accelerated* VM, this goes 500 MB/s, three times as fast as before.
+    echo "* Writing the GuestOS image to ${LV}"
+    dd if="${TMPDIR}/disk.img" of=${LV} bs=10M conv=sparse status=progress


@andrewbattat Do you remember why you chose pv over dd? I can't get into GitLab to see the discussion on the original MR.

2f0db1c
It was for the sake of the installation progress bar (my first real node team task 🥲)

But now that the installation is so much faster, maybe the progress bar isn't necessary

Either way, this change should be in a separate PR in my opinion.

Bownairo · 2024-10-03T22:15:58Z

ic-os/defs.bzl

+truncate -s 128G target.img
+qemu-system-x86_64 -machine type=q35,accel=kvm -enable-kvm -nographic -m 4G -bios /usr/share/ovmf/OVMF.fd -device vhost-vsock-pci,guest-cid=\\$$CID -boot d -drive file=target.img,format=raw,if=virtio -drive file=disk.img,format=raw,if=virtio -netdev user,id=user.0,hostfwd=tcp::2222-:22 -device virtio-net,netdev=user.0


This same tooling is used for testing HostOS and GuestOS as well. If you'd like to add this for testing SetupOS, could you add a branch to the logic? We have a similar thing above for the remote VMs, in setting the nested flag.

These changes aren't really related to the firewall feature, could you please move them to a separate PR?

Bownairo · 2024-10-03T22:16:02Z

ic-os/defs.bzl

 cd \\$$TEMP
-tar xf disk-img.tar
-qemu-system-x86_64 -machine type=q35,accel=kvm -enable-kvm -nographic -m 4G -bios /usr/share/ovmf/OVMF.fd -device vhost-vsock-pci,guest-cid=\\$$CID -drive file=disk.img,format=raw,if=virtio -netdev user,id=user.0,hostfwd=tcp::2222-:22 -device virtio-net,netdev=user.0
+tar xSf $$IMAGE


I don't think GNU tar that we use in devenvs/build container supports sparse extraction.

This option is meaningful only when creating or updating archives. It has no effect on extraction.

https://www.gnu.org/software/tar/manual/html_node/sparse.html

DFINITYManu changed the title ~~DRE-252: First phase of the firewall setup feature~~ feat(icos) DRE-252: First phase of the firewall setup feature Sep 11, 2024

DFINITYManu changed the title ~~feat(icos) DRE-252: First phase of the firewall setup feature~~ feat(icos): First phase of the firewall setup feature (DRE-258) Sep 11, 2024

github-actions bot added the feat label Sep 11, 2024

DFINITYManu force-pushed the firewallingsetupos branch 8 times, most recently from 31603d7 to ca53f81 Compare September 12, 2024 15:46

andrewbattat reviewed Sep 12, 2024

View reviewed changes

ic-os/docs/Network-Configuration.adoc Show resolved Hide resolved

DFINITYManu force-pushed the firewallingsetupos branch 2 times, most recently from 2c44825 to c849b6e Compare September 13, 2024 17:22

DFINITYManu requested a review from andrewbattat September 13, 2024 17:22

DFINITYManu force-pushed the firewallingsetupos branch 5 times, most recently from 773291f to f9c86eb Compare September 13, 2024 18:32

DFINITYManu force-pushed the firewallingsetupos branch 2 times, most recently from 9121ce5 to 0ec762c Compare September 13, 2024 19:00

andrewbattat reviewed Sep 13, 2024

View reviewed changes

rs/ic_os/config/src/main.rs Outdated Show resolved Hide resolved

DFINITYManu force-pushed the firewallingsetupos branch from 0ec762c to 9b84e0c Compare September 13, 2024 19:58

DFINITYManu force-pushed the firewallingsetupos branch 2 times, most recently from 78386c8 to 7521ca4 Compare September 26, 2024 09:49

DFINITYManu force-pushed the firewallingsetupos branch from 156c067 to fa9a7b9 Compare October 3, 2024 13:13

Placate the shfmt gods.

1e50c99

DFINITYManu marked this pull request as ready for review October 3, 2024 14:04

DFINITYManu requested review from a team as code owners October 3, 2024 14:04

github-actions bot added @consensus @node @ic-owners-owners @idx labels Oct 3, 2024

marko-k0 approved these changes Oct 3, 2024

View reviewed changes

Fix unset variable check.

3350c6a

DFINITYManu added 8 commits October 3, 2024 16:55

Ensure that the /config firewall.json file is copied to /var/ic/confi…

305446e

…g because who knows thought this was a good idea.

Small comment fix.

c1c2c8b

Permit skips of network configuration checks.

0550117

Indicate where HostOS is being installed.

915f143

Enhance the kernel command line function checkers to accept valueless…

1e9628c

… options.

Add kernel command line option to stop installation.

083adfd

Document SetupOS kernel command line options.

97ad6a2

Note that installation was stopped when kernel command option was spe…

4617823

…cified.

andrewbattat reviewed Oct 3, 2024

View reviewed changes

ic-os/components/setupos-scripts/check-network.sh Show resolved Hide resolved

andrewbattat reviewed Oct 3, 2024

View reviewed changes

DFINITYManu added 4 commits October 3, 2024 18:32

Fix a few things.

84febae

Provide knob to prevent reboots after installation, to inspect the re…

8eb0942

…sults interactively after installation.

Bownairo requested changes Oct 3, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(icos): First phase of the firewall setup feature (DRE-258) #1451

feat(icos): First phase of the firewall setup feature (DRE-258) #1451

DFINITYManu commented Sep 11, 2024 •

edited by sasa-tomic

Loading

DFINITYManu commented Sep 13, 2024 •

edited

Loading

github-actions bot commented Sep 26, 2024

venkkatesh-sekar commented Sep 26, 2024 •

edited

Loading

DFINITYManu commented Oct 3, 2024

andrewbattat commented Oct 3, 2024

andrewbattat Oct 3, 2024

DFINITYManu Oct 3, 2024

DFINITYManu Oct 3, 2024

andrewbattat Oct 3, 2024

DFINITYManu Oct 3, 2024

andrewbattat Oct 3, 2024

andrewbattat Oct 3, 2024

andrewbattat Oct 3, 2024

andrewbattat Oct 3, 2024

Bownairo Oct 3, 2024

andrewbattat Oct 3, 2024 •

edited

Loading

Bownairo Oct 3, 2024

Bownairo Oct 3, 2024

	let _config_ = firewall_json::get_firewall_rules_json_or_default(
	firewall_json::get_firewall_rules_json_or_default(

	use std::path::Path;
	use std::path::PathBuf;
	use std::path::{Path,PathBuf};

		truncate -s 128G target.img
		qemu-system-x86_64 -machine type=q35,accel=kvm -enable-kvm -nographic -m 4G -bios /usr/share/ovmf/OVMF.fd -device vhost-vsock-pci,guest-cid=\\$$CID -boot d -drive file=target.img,format=raw,if=virtio -drive file=disk.img,format=raw,if=virtio -netdev user,id=user.0,hostfwd=tcp::2222-:22 -device virtio-net,netdev=user.0

feat(icos): First phase of the firewall setup feature (DRE-258) #1451

Are you sure you want to change the base?

feat(icos): First phase of the firewall setup feature (DRE-258) #1451

Conversation

DFINITYManu commented Sep 11, 2024 • edited by sasa-tomic Loading

DFINITYManu commented Sep 13, 2024 • edited Loading

github-actions bot commented Sep 26, 2024

venkkatesh-sekar commented Sep 26, 2024 • edited Loading

DFINITYManu commented Oct 3, 2024

andrewbattat commented Oct 3, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andrewbattat Oct 3, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DFINITYManu commented Sep 11, 2024 •

edited by sasa-tomic

Loading

DFINITYManu commented Sep 13, 2024 •

edited

Loading

venkkatesh-sekar commented Sep 26, 2024 •

edited

Loading

andrewbattat Oct 3, 2024 •

edited

Loading